-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Task terminals with a failed process
task aren't always reused properly
#141939
Conversation
@@ -1412,7 +1412,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { | |||
} | |||
|
|||
// First onExit to consumers, this can happen after the terminal has already been disposed. | |||
this._onExit.fire(this._exitCode); | |||
this._onExit.fire(parsedExitResult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of null
seems pretty confusing, could this instead fire exitCodeOrError
which would let you check message
directly? If exitCodeOrError
is an ITerminalLaunchError
then the terminal failed before the process launched which is the case you're after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message
is localized, so I'm not sure what I should check for there.
Checking if exitCodeOrError
is an ITerminalLaunchError
will work though. I didn't realize it would only be an ITerminalLaunchError
when the terminal failed before the process launched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parsedExitResult
is localized for the notification, exitCodeOrError.message
isn't though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Thanks for reviewing! |
Fixes #141728